Skip to content

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Feb 5, 2025

Flag (target modifier) to mitigate against straight line speculation (SLS).
-Zharden-sls=[none|all|return|indirect-jmp].

The flag enables the related features: +harden-sls-ijmp,+harden-sls-ret.
The flag is tracked as a target modifier to be equal between linked crates.

Tracking issue: #116851

@rustbot
Copy link
Collaborator

rustbot commented Feb 5, 2025

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2025
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2025

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@klensy
Copy link
Contributor

klensy commented Feb 25, 2025

First commit adds retpoline feature, but PR descriptions says about sls.

@azhogin azhogin marked this pull request as draft February 25, 2025 18:23
@azhogin
Copy link
Contributor Author

azhogin commented Feb 25, 2025

First commit adds retpoline feature, but PR descriptions says about sls.

Yes, first commit is in another PR #135927.
This one depends on.

@Dylan-DPC
Copy link
Member

Marking this as blocked on #135927

@Dylan-DPC Dylan-DPC added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2025
@Darksonn
Copy link
Contributor

I filed an MCP for this flag: rust-lang/compiler-team#869

@bors
Copy link
Collaborator

bors commented May 18, 2025

☔ The latest upstream changes (presumably #141232) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines 9 to 14
#![feature(no_core, lang_items)]
#![no_std]
#![no_core]

#[lang = "sized"]
pub trait Sized {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use add-core-stubs, don't handwrite the minicore.

Comment on lines 1 to 6
warning: target feature `retpoline-external-thunk` cannot be enabled with `-Ctarget-feature`: use `x86-retpoline` target modifier flag instead
|
= note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>

warning: 1 warning emitted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this possible at all?

//@ compile-flags: --target=x86_64-unknown-linux-gnu --crate-type=lib
//@ needs-llvm-components: x86
//@ [by_flag]compile-flags: -Zharden-sls=all
//@ [by_feature]compile-flags: -Ctarget-feature=+harden-sls-ijmp,+harden-sls-ret
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should not be set by -Ctarget-feature.

@workingjubilee workingjubilee self-assigned this Jun 14, 2025
@ojeda
Copy link
Contributor

ojeda commented Jul 16, 2025

@azhogin Since #135927 is merged now, should this be rebased and unblocked now?

Thanks!

@Darksonn Darksonn added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 16, 2025
@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jul 20, 2025
@azhogin azhogin marked this pull request as ready for review July 20, 2025 11:25
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 20, 2025
@azhogin
Copy link
Contributor Author

azhogin commented Jul 20, 2025

Converted to "Ready for review" because retpoline PR was merged.

@wesleywiser
Copy link
Member

Adding myself to review as part of the RfL project goal I signed up for 🙂

r? wesleywiser

@ojeda
Copy link
Contributor

ojeda commented Aug 21, 2025

Thanks Wesley! Much appreciated.

@ojeda
Copy link
Contributor

ojeda commented Aug 22, 2025

By the way, @azhogin, I think we should have an assembly test for this one too, even if small/trivial, to double-check LLVM is actually doing something, like in the other flags.

(Another rebase is needed, given the recent renaming of the folders)

Thanks!

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@azhogin
Copy link
Contributor Author

azhogin commented Aug 27, 2025

By the way, @azhogin, I think we should have an assembly test for this one too, even if small/trivial, to double-check LLVM is actually doing something, like in the other flags.

(Another rebase is needed, given the recent renaming of the folders)

Thanks!

Added tests/assembly-llvm/x86_64-sls.rs test for x86 asm codegen

Comment on lines 61 to 63
4 => unsafe { return std::intrinsics::unchecked_div(b, a) + 4 },
5 => unsafe { return std::intrinsics::unchecked_div(b, a) + 5 },
6 => unsafe { return std::intrinsics::unchecked_div(b, a) + 6 },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume all these are here to ensure the compiler is likely to generate a jump table?

By the way, you can remove all the returns, and it will be a bit more readable since the formatting will be redone.

Comment on lines 29 to 33
if a > 0 {
return unsafe { std::intrinsics::unchecked_div(a, b) };
} else {
return unsafe { std::intrinsics::unchecked_div(b, a) };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if a > 0 {
return unsafe { std::intrinsics::unchecked_div(a, b) };
} else {
return unsafe { std::intrinsics::unchecked_div(b, a) };
}
if a > 0 {
unsafe { std::intrinsics::unchecked_div(a, b) }
} else {
unsafe { std::intrinsics::unchecked_div(b, a) }
}

@ojeda
Copy link
Contributor

ojeda commented Aug 27, 2025

Added tests/assembly-llvm/x86_64-sls.rs test for x86 asm codegen

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.